-
Notifications
You must be signed in to change notification settings - Fork 95
tutorial: replace deprecated DataPoint API #7044
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: 8.6.x
Are you sure you want to change the base?
Conversation
d109fc3 to
f684f1d
Compare
cylc/flow/etc/tutorial/cylc-forecasting-workflow/bin/get-rainfall
Outdated
Show resolved
Hide resolved
cylc/flow/etc/tutorial/cylc-forecasting-workflow/bin/consolidate-observations
Show resolved
Hide resolved
cylc/flow/etc/tutorial/cylc-forecasting-workflow/bin/get-observations
Outdated
Show resolved
Hide resolved
|
@wxtim, FYI, this will need to go into a bugfix release. |
oliver-sanders
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
PR is on the wrong branch.
Some small comments so far:
cylc/flow/etc/tutorial/cylc-forecasting-workflow/bin/get-observations
Outdated
Show resolved
Hide resolved
cylc/flow/etc/tutorial/cylc-forecasting-workflow/lib/python/util.py
Outdated
Show resolved
Hide resolved
|
It looks like we have multiple options for reading HDF5 files. h5py, is one, but Pandas and xarray are apparently alternatives. Pandas might be an appealing option as widely used and is already an optional dependency. Can we have a quick review of the options to work out which is the most lightweight, easiest to support, least likely to cause problems, etc. |
I think that they may all be the same: The documentation certainly suggests that pandas.read_hdf uses pytables, whose docs suggest that uses h5py. Looking at the source of pandas shows h5py is an optional depency. I think it's going to be h5py whatever we choose. |
|
The Note: It uses ~25% CPU (but negligible RAM) on my box. |
cylc/flow/etc/tutorial/cylc-forecasting-workflow/bin/get-rainfall
Outdated
Show resolved
Hide resolved
cylc/flow/etc/tutorial/cylc-forecasting-workflow/bin/get-rainfall
Outdated
Show resolved
Hide resolved
oliver-sanders
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM.
Tested as working.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Might need to delete some commits post base change.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@wxtim, soz, this branch still has 8.7.x commits post rebase.
|
Looking good, assigned @ChrisPaulBennett for a quick skim. Note, we will need followup PRs in:
|
e984c2e to
1351a9d
Compare


Closes #6053
Datapoint is to be switched off from the first of December.
The data is available from the Met Office via the Amazon Sustainability Data Initiative. Amazon link.
Annoyingly the metadata there seems quite limited, so I applied to a bunch of contacts in Obs R&D who insisted that the data has the following properties:
Which raises 2 problems:
Rather than re-jigging the mathematics I've made something vaguely plausible by fiddling with domain values. Hopefully this creates a product good enough for the training purpose for which it is built.
Finally, I have fixed a bug I introduced when I added the SYNOP collecting routine - these wind observations are in meteorological convention (wind is blowing from), but we need where the wind is going to, so all wind directions were 180° off!
CONTRIBUTING.mdand added my name as a Code Contributor.setup.cfg(andconda-environment.ymlif present).?.?.xbranch.